`CLI`: add option `--clean-workdir` to `verdi node delete` #6756

khsrali · 2025-02-14T18:34:44Z

Implements #6732 also solves #4693.

Like the tiles says, now, with verdi node delete --clean-workdir one can also remove all remote directories of workflows and workchains.

codecov · 2025-02-14T18:53:03Z

Codecov Report

Attention: Patch coverage is 98.07692% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.19%. Comparing base (f4c55f5) to head (8555370).

Files with missing lines	Patch %	Lines
src/aiida/orm/utils/remote.py	93.75%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6756      +/-   ##
==========================================
+ Coverage   78.15%   78.19%   +0.05%     
==========================================
  Files         564      564              
  Lines       42643    42677      +34     
==========================================
+ Hits        33322    33367      +45     
+ Misses       9321     9310      -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

superstar54

Hi @khsrali , thanks for addressing the issues. I added a few comments on the code. I am not familiar with tests in aiida-core, so no comments on that part.

superstar54 · 2025-02-14T20:33:49Z

src/aiida/cmdline/commands/cmd_node.py

+@click.option(
+    '--clean-workdir',
+    is_flag=True,
+    help='Also clean the remote work directory. (only applies to workflows and CalcJobNodes)',


only applies to workflows and CalcJobNodes

It is better to revise this message because this applies to any node, as a node can be an input of the workchain or calcjob.

e.g., but could be improved.

Suggested change

help='Also clean the remote work directory. (only applies to workflows and CalcJobNodes)',

help='Also clean the remote work directories associated with the nodes.

The point I wanted to make clear, is that doesn't apply if one pass a RemoteData node.
Just to respect traversal rule, and to make it easier, RemoteData directories can only be cleaned if the parent calcjob is passed to the function.

The point I wanted to make clear, is that doesn't apply if one pass a RemoteData node.

I think it will also work if you pass a RemoteData node, because the associated process node will be deleted thus the RemoteData node itself will be cleaned.

Yes, you are right. I can improve the help message.

superstar54 · 2025-02-14T20:47:46Z

src/aiida/cmdline/commands/cmd_node.py

+                    user = User.collection.get_default()
+                    counter = 0
+                    for computer_uuid, paths in path_mapping.items():
+                        computer = load_computer(uuid=computer_uuid)
+                        transport = AuthInfo.collection.get(
+                            dbcomputer_id=computer.pk, aiidauser_id=user.pk
+                        ).get_transport()
+
+                        with transport:
+                            for remote_folder in paths:
+                                remote_folder._clean(transport=transport)
+                                counter += 1


Similar code is used in calcjob_cleanworkdir. Consider refactoring into a reusable function to avoid duplication.

Yeah, I wanted modularize, but then I thought it's also used only in these two functions (here and calcjob_cleanworkdir), Also it's specific codeme, I'm not sure if it worth to modularize it. 🤔

It's good to modularize as much as we can 😆 .

I mean it reduces the readability. If it's only for two use case.
But ok, I'll see what I can do. ;)

superstar54 · 2025-02-14T20:48:42Z

src/aiida/cmdline/commands/cmd_node.py

+
+                        with transport:
+                            for remote_folder in paths:
+                                remote_folder._clean(transport=transport)


Check if the remote folder is already cleaned or not, either here or inside the _clean method.

That already been done via flag only_not_cleaned:

path_mapping = get_calcjob_remote_paths(calcjobs_pks, only_not_cleaned=True,)

superstar54 · 2025-02-14T20:52:20Z

src/aiida/cmdline/commands/cmd_node.py

+                        'Remote directories of nodes with the following pks would be cleaned: '
+                        + ' '.join(map(str, descendant_pks))
+                    )
+                    click.confirm('Shall I continue?', abort=True)


The command asks for confirmation twice: 1) cleaning remote folders and 2) deleting nodes. I would prefer a single confirmation prompt by merging the messages into one.

The main reason I did this, was to provide additional flexibility so to only clean Remote directories without actually deleting the nodes.
For instance in #4693 that might be useful.

I see, it makes sense to me. 👍

unkcpz

See my comment in the original OP, I think it should not add to verdi node.

For the implementation, to avoid multi-layer of if..else, please consider using early return.

unkcpz · 2025-02-15T10:07:43Z

src/aiida/cmdline/commands/cmd_node.py

+        if not calcjobs_pks:
+            echo.echo_report('--clean-workdir ignored. No CalcJobNode associated with the given node, found.')


I think you can do early return here to avoid nested if..else.

unkcpz · 2025-02-15T10:07:57Z

src/aiida/cmdline/commands/cmd_node.py

+            if not path_mapping:
+                echo.echo_report('--clean-workdir ignored. CalcJobNode work directories are already cleaned.')


Early return here as well?

unkcpz · 2025-02-15T10:08:16Z

src/aiida/cmdline/commands/cmd_node.py

+            else:
+                descendant_pks = [remote_folder.pk for paths in path_mapping.values() for remote_folder in paths]
+
+                if not force and not dry_run:


Early return?

unkcpz · 2025-02-15T10:08:28Z

src/aiida/cmdline/commands/cmd_node.py

+                    )
+                    click.confirm('Shall I continue?', abort=True)
+
+                if dry_run:


early return as well?

khsrali · 2025-02-15T10:57:39Z

@unkcpz Thanks for your suggestions,

For the implementation, to avoid multi-layer of if..else, please consider using early return.

Early return had to be avoided, because --clean-workdir is an additional flag, the command should continue executing even if there is no directories to clean. And cleaning has to come before deleting nodes.

For more detailed behaviour, please also have a look at the tests. All scenarios and the expected behaviour are pictured, there.

In aiida-core, at the moment, only calcjob and workchain will have remote resource that need to cleaned so I think it >makes no sense to add functionality to add --clean-remote-folder to all node type since there are tons of other node type >that has no remote folder attached. Even if you decide to add this to aiida-core. will it make more sense to add to verdi >process subcommand?

The only fact it doesn't apply to add nodes, is not an issue itself, because verdi node delete already supports options that are specific to wrokflow nodes, for example:

  --call-calc-forward / --no-call-calc-forward
                                  Whether to expand the node set by following
                                  CALL links to calculations forwards.
                                  [default: call-calc-forward]
  --call-work-forward / --no-call-work-forward
                                  Whether to expand the node set by following
                                  CALL links to workflows forwards.  [default:
                                  call-work-forward]

Anyways, I like your idea of having it as a subcommand of verdi process, however that introduces more challenge, as the process might already being run so first has to be do a verdi process kill, etc.
And tbh this feature is more of a node feature, so that it cleans remote files as well, while deleting a node. So I more inclined to @superstar54 suggestion that it fits more to verdi node delete

unkcpz · 2025-02-15T20:29:22Z

Early return had to be avoided, because --clean-workdir is an additional flag

I believe you can ;) Just wrap the block inside if clean_workchain into a function and ask chatgpt how to do early return. The happy path of the code is bit more clear.

For more detailed behaviour, please also have a look at the tests.

Nice.

because verdi node delete already supports options that are specific to wrokflow nodes, for example:

You may misunderstood here for --call-calc-forward and --call-work-forward. This is from graph_traversal_rule and apply for nodes that along the traverse. However, I am talking about the node (the pk or uuid) you passed to the command, the clean_workdir is ONLY applied to workchain and calcjob.

unkcpz · 2025-02-15T20:46:04Z

src/aiida/cmdline/commands/cmd_node.py

@@ -328,9 +328,14 @@ def extras(nodes, keys, fmt, identifier, raw):
 @click.argument('identifier', nargs=-1, metavar='NODES')
 @options.DRY_RUN()
 @options.FORCE()
+@click.option(
+    '--clean-workdir',
+    is_flag=True,


Put default to True is backward incompatible and potentially danger.

Well, that's exactly what this command does :-)
It define it as a flag with is_flag=True so the default value is False

Ah, right. Then could you write default=False to be more explicit?

unkcpz · 2025-02-15T20:48:38Z

however that introduces more challenge, as the process might already being run so first has to be do a verdi process kill, etc.

I don't get want you mean, sorry. Put the subcommand in verdi node didn't solve the issue you mentioned. The process can still run but your verdi node delete can still called without kill the process first, no?

khsrali · 2025-02-17T08:38:08Z

You may misunderstood here for --call-calc-forward and --call-work-forward. This is from graph_traversal_rule and apply >for nodes that along the traverse. However, I am talking about the node (the pk or uuid) you passed to the command, the >clean_workdir is ONLY applied to workchain and calcjob.

I understand, but traversal rules affects only descendants of workchain and calcjob. As it has no effect or whatsoever on individual RemoteData nodes for example. The affect only appears if a calcjob node that includes that RemoteData has to be deleted according to the given traversal rules. To be clear:
verdi node delete <RemoteData> --call-calc-forward is exactly identical to:
verdi node delete <RemoteData>

In the exact same way, '--clean-workdir' has no effect on RemoteData nodes, when specified individually:
verdi node delete <RemoteData> --clean-workdir is exactly identical to:
verdi node delete <RemoteData

khsrali · 2025-02-17T08:44:55Z

I don't get want you mean, sorry. Put the subcommand in verdi node didn't solve the issue you mentioned. The process >can still run but your verdi node delete can still called without kill the process first, no?

True, but I feel like that's the expected behavior of verdi node delete "delete the node no matter what's happening or what's the stage" in fact, it already does that. It does not care at all, if the process is running or whatever.

While for verdi process that's not the case. It's expected behavior is different, one supposed to
pause, play, repair a process. Is not meant to support a "reckless" command, in my opinion.

superstar54 · 2025-02-17T09:47:22Z

I agree with @khsrali ,

verdi process command is designed to interact with Process (most of the time, the living Process), not the Node.
because of the graph_traversal_rule, the --clean-workdir flag works not only for WorkChain node, but for all node type.

…cleanworkdir

khsrali · 2025-02-18T19:33:25Z

@superstar54 applied your review. Now, it's more modular, and more readable.
Please let me know if you have more concerns

superstar54 · 2025-02-19T06:31:50Z

Please let me know if you have more concerns

thanks for the work, LGTM!

verdi node delete --clean workdir

0a200c3

khsrali linked an issue Feb 14, 2025 that may be closed by this pull request

Add option to clean remote folder when deleting nodes #6732

Open

khsrali mentioned this pull request Feb 14, 2025

verdi calcjob cleanworkdir: Add option for work chain #4693

Open

khsrali requested review from superstar54 and GeigerJ2 February 14, 2025 18:46

.

3374580

superstar54 requested changes Feb 14, 2025

View reviewed changes

unkcpz requested changes Feb 15, 2025

View reviewed changes

khsrali mentioned this pull request Feb 15, 2025

Add ArithmeticAdd CJ Node fixture without run or submit #6733

Open

unkcpz requested changes Feb 15, 2025

View reviewed changes

khsrali added 2 commits February 18, 2025 19:08

Merge remote-tracking branch 'upstream/main' into verdi/node-delete--…

60b4ac1

…cleanworkdir

review applied

8555370

khsrali mentioned this pull request Feb 20, 2025

Add option to clean remote folder when deleting nodes #6732

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`CLI`: add option `--clean-workdir` to `verdi node delete` #6756

`CLI`: add option `--clean-workdir` to `verdi node delete` #6756

khsrali commented Feb 14, 2025

codecov bot commented Feb 14, 2025 •

edited

Loading

superstar54 left a comment

superstar54 Feb 14, 2025

khsrali Feb 15, 2025

superstar54 Feb 17, 2025

khsrali Feb 17, 2025

superstar54 Feb 14, 2025

khsrali Feb 15, 2025

superstar54 Feb 17, 2025

khsrali Feb 17, 2025

superstar54 Feb 14, 2025 •

edited

Loading

khsrali Feb 15, 2025

superstar54 Feb 17, 2025

superstar54 Feb 14, 2025

khsrali Feb 15, 2025

superstar54 Feb 17, 2025

unkcpz left a comment

unkcpz Feb 15, 2025

unkcpz Feb 15, 2025

unkcpz Feb 15, 2025

unkcpz Feb 15, 2025

khsrali commented Feb 15, 2025

unkcpz commented Feb 15, 2025 •

edited

Loading

unkcpz Feb 15, 2025 •

edited

Loading

khsrali Feb 17, 2025

unkcpz Feb 18, 2025

unkcpz commented Feb 15, 2025

khsrali commented Feb 17, 2025

khsrali commented Feb 17, 2025

superstar54 commented Feb 17, 2025

khsrali commented Feb 18, 2025

superstar54 commented Feb 19, 2025

	help='Also clean the remote work directory. (only applies to workflows and CalcJobNodes)',
	help='Also clean the remote work directories associated with the nodes.

		if not calcjobs_pks:
		echo.echo_report('--clean-workdir ignored. No CalcJobNode associated with the given node, found.')

		if not path_mapping:
		echo.echo_report('--clean-workdir ignored. CalcJobNode work directories are already cleaned.')

CLI: add option --clean-workdir to verdi node delete #6756

Are you sure you want to change the base?

CLI: add option --clean-workdir to verdi node delete #6756

Conversation

khsrali commented Feb 14, 2025

codecov bot commented Feb 14, 2025 • edited Loading

Codecov Report

superstar54 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superstar54 Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khsrali commented Feb 15, 2025

unkcpz commented Feb 15, 2025 • edited Loading

unkcpz Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unkcpz commented Feb 15, 2025

khsrali commented Feb 17, 2025

khsrali commented Feb 17, 2025

superstar54 commented Feb 17, 2025

khsrali commented Feb 18, 2025

superstar54 commented Feb 19, 2025

`CLI`: add option `--clean-workdir` to `verdi node delete` #6756

`CLI`: add option `--clean-workdir` to `verdi node delete` #6756

codecov bot commented Feb 14, 2025 •

edited

Loading

superstar54 Feb 14, 2025 •

edited

Loading

unkcpz commented Feb 15, 2025 •

edited

Loading

unkcpz Feb 15, 2025 •

edited

Loading